Skip to content

[DependencyInjection] Fix tagged service priority inconsistent method name #14487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 28, 2020
Merged

[DependencyInjection] Fix tagged service priority inconsistent method name #14487

merged 1 commit into from
Oct 28, 2020

Conversation

yann-eugone
Copy link
Contributor

The service priority default_priority_method contains an inconsistent method name regarding code example.

@wouterj
Copy link
Member

wouterj commented Oct 26, 2020

Hi @yann-eugone!

This is intended, the getDefaultPriority is the default value for the default_priority_method option. You can use this option to use another method name (e.g. getPriority).

Maybe we should emphasize this by improving the sentence in between the code examples. For instance, like this:

If you want to have another method defining the priority (e.g. ``getPriority()``),
you can define it in the configuration of the collecting service:

If you agree, can you please update this PR/create a new PR with this change?

@yann-eugone
Copy link
Contributor Author

Thank you @wouterj ! :)

Look like I read the docblock to fast. But yeah, I think this part of documentation can be more clear with some minor changes.
Will apply your suggestion (and remove my changes)

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick actions!

@yann-eugone
Copy link
Contributor Author

Does the pipeline failure has something to do with the changes we introduced ?

@wouterj
Copy link
Member

wouterj commented Oct 26, 2020

No, I see we have an error in the 5.1 version of the docs. It's not related to your changes

@javiereguiluz javiereguiluz added this to the 4.4 milestone Oct 28, 2020
@javiereguiluz
Copy link
Member

Thank you Yann! We merged this in 4.4 branch and all the other upper branches.

@javiereguiluz javiereguiluz merged commit a895c0f into symfony:4.4 Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants